Skip to content

Fix #1307: reset config.theme to default + document composer install on v2.0 upgrade - #1317

Merged
rumblefrog merged 1 commit into
mainfrom
fix/issue-1307-upgrade-default-theme
May 10, 2026
Merged

Fix #1307: reset config.theme to default + document composer install on v2.0 upgrade#1317
rumblefrog merged 1 commit into
mainfrom
fix/issue-1307-upgrade-default-theme

Conversation

@rumblefrog

@rumblefrog rumblefrog commented May 10, 2026

Copy link
Copy Markdown
Member

Fixes #1307.

Summary

v2.0 ships a complete chrome rewrite (#1123 / #1207 / #1259 / #1275).
Operators upgrading from v1.x hit two real-world UX gaps the dev stack
doesn't catch (it always starts fresh on default and always has a
populated vendor/). Both halves land here:

  • Half 1 — paired updater migration web/updater/data/808.php:
    rewrites :prefix_settings.config.theme back to default for any
    install not already on the shipped theme. The updater wizard itself
    runs against default (IS_UPDATE override in init.php), but
    that scoping ends when the operator clicks "Return to panel" — the
    next request reads the stale fork value and Smarty fatals against
    v2.0 templates the v1.x fork doesn't contain. Idempotent (the
    WHERE clause excludes the already-set row), safe to re-run, and
    the default value matches the seed in
    install/includes/sql/data.sql so fresh and upgraded installs
    converge. Operators with a fork re-select it from Admin →
    Settings → Themes once they've ported it (per Author UPGRADING.md for 1.x to 2.0 #1115).
  • Half 2 — composer install prerequisite documented:
    UPGRADING.md gains a new "PHP dependencies" section above the
    existing telemetry one, plus a "Theme compatibility" section that
    surfaces half 1 for docs-first readers. README.md gets a
    one-line cross-reference at the head of the master-branch install
    instructions so admins reading those don't miss UPGRADING.md.
    v2.0's autoload check only verifies vendor/autoload.php exists,
    not that it's v2.0-shaped — so panels with a stale v1.x vendor/
    pass bootstrap and 500 mid-render with the actual class-not-found
    line buried in the PHP error log.

Per AGENTS.md "Updater migrations": idempotent UPDATE, :prefix_
placeholder, // @phpstan-ignore variable.undefined on each
$this->dbs call, defaults converge with data.sql, registered in
store.json. Per "Keep the docs in sync": UPGRADING.md is the
load-bearing operator surface for upgrade-day defects, so the docs
land in the same PR as the migration.

Test plan

  • ./sbpp.sh phpstan → no errors (no new violations introduced).
  • ./sbpp.sh test --filter=UpgradeThemeReset → 4/4 pass:
    • testForkThemeIsResetToDefault — fork value rewritten on first run.
    • testInstallAlreadyOnDefaultIsUntouched — WHERE clause excludes
      the already-set row.
    • testRerunImmediatelyAfterFirstPassIsNoOp — second run matches
      zero rows (idempotency guarantee).
    • testMigrationDefaultMatchesDataSqlSeed — the migration's
      'default' literal matches data.sql's ('config.theme', 'default') seed; future renames of the shipped theme directory
      won't silently diverge fresh and upgraded installs.
  • ./sbpp.sh test (full suite) → 407 tests, 1778 assertions, all OK.

ts-check / composer api-contract / e2e weren't run because this
PR doesn't touch web/scripts/, web/themes/*/js/, web/api/handlers/,
or any user-facing UI surface.

@rumblefrog
Fix #1307: reset config.theme to default + document composer install …
facc3da 
…on v2.0 upgrade

v2.0 ships a complete chrome rewrite (#1123 / #1207 / #1259 / #1275).
Operators upgrading from v1.x hit two real-world UX gaps the dev stack
doesn't catch (it always starts fresh on `default` and always has a
populated `vendor/`):

1. `config.theme` carries the v1.x fork's directory name out of the
   updater. The updater wizard itself runs against `default` (the
   `IS_UPDATE` override in `init.php`), but that scoping ends when the
   operator clicks "Return to panel" and the next request reads the
   stale fork value. The v2.0 default theme has new typed View DTOs,
   a new admin sidebar partial, and rewritten template signatures the
   v1.x fork literally does not contain — best case Smarty fatals,
   worst case it half-renders against undefined variables.

2. `vendor/` is present but stale on git-based upgrades. v2.0 added
   symfony/mailer, league/commonmark, and major-bumped lcobucci/jwt
   and Smarty. `init.php`'s autoload check only verifies the file
   exists, not that it's v2.0-shaped — so upgraded panels pass
   bootstrap and 500 mid-render with the actual class-not-found line
   buried in the PHP error log.

Half 1: paired updater migration `web/updater/data/808.php` rewrites
`config.theme` back to `default` for any install not already on the
shipped theme. Idempotent (the WHERE clause excludes the already-set
row), safe to re-run, and the default value matches the seed in
`install/includes/sql/data.sql` so fresh and upgraded installs
converge. Operators with a fork re-select it from Admin → Settings →
Themes once they've ported it (per #1115).

Half 2: documents the `composer install` prerequisite in UPGRADING.md
above the existing telemetry section, plus a new "Theme compatibility"
section that surfaces the half-1 reset to operators reading docs
ahead of the upgrade. README.md gains a one-line cross-reference at
the head of the master-branch install instructions so admins reading
those instructions don't miss UPGRADING.md.

Regression coverage: `web/tests/integration/UpgradeThemeResetTest.php`
exercises four properties — fork value rewritten, default value left
untouched, re-run is a no-op, and migration's default matches the
data.sql seed.
@rumblefrog
rumblefrog added this pull request to the merge queue May 10, 2026
Merged via the queue into main with commit 3a672ab May 10, 2026
4 checks passed
@rumblefrog
rumblefrog deleted the fix/issue-1307-upgrade-default-theme branch May 10, 2026 18:45
@rumblefrog

rumblefrog commented May 10, 2026

Copy link
Copy Markdown
Member Author

Post-merge adversarial review (PR was already merged when I ran). One real finding plus a sweep of the rest.

What checks out

  • Migration shape per AGENTS.md "Updater migrations": idempotent UPDATE (WHERE excludes already-default rows), :prefix_ placeholder, // @phpstan-ignore variable.undefined on each $this->dbs call, registered as "808": "808.php" (next integer above 807, no clash with feat(telemetry): add anonymous opt-out daily telemetry ping (#1126) #1300's telemetry migration).
  • 'default' literal matches data.sql's seed; testMigrationDefaultMatchesDataSqlSeed locks that parity.
  • Tests: 4/4 pass on the new file, 407/1778 on full suite. Confirmed adversarially — gutting the migration body to <?php return true; fails 3/4 (the no-op-on-default case trivially passes), and restoring it gets back to green.
  • PHPStan: 229/229 files, no errors.
  • Edge cases I poked at: empty string → reset (correct), case-variant like 'Default' → not reset (collation-dependent, pre-existing concern not introduced here), NULL → impossible (value column is NOT NULL), case where operator runs migration on a fresh install that already has default → no-op via WHERE.
  • Fork-theme directory left in place on disk — consistent with the doc's "your work isn't lost" framing.
  • The aggressive (always-reset) approach matches the issue body's preference and the docs explain the re-select path clearly.

Finding: doc inaccuracy in UPGRADING.md

The new "PHP dependencies" section claims:

Release tarballs ship a fresh `web/includes/vendor/` so tarball upgrades work out of the box. Git-based upgrades ... need the explicit `composer install` above.

This is factually incorrect for the current release pipeline. Verified against the live 1.8.4 release zip:

```
$ curl -sL https://github.com/sbpp/sourcebans-pp/releases/download/1.8.4/sourcebans-pp-1.8.4.webpanel-only.zip -o /tmp/x.zip
$ python3 -c "import zipfile; z=zipfile.ZipFile('/tmp/x.zip'); print(f'total: {len(z.namelist())}, vendor entries: {sum(1 for n in z.namelist() if \"vendor\" in n)}')"
total: 985, vendor entries: 0
```

Root cause: `.github/workflows/release.yml`'s `build-webpanel` job (lines 62-79) does `cp -R web /tmp/${pkg}` and zips, with no `composer install` step. `web/includes/vendor` is gitignored (.gitignore line 65), so the workflow's checkout doesn't have it and the zip doesn't either. This has been the case since the workflow's introduction in #1065 (May 2026), so all 1.8.x release zips lack vendor too — `README.md`'s "release version come bundled with all required code dependencies" claim has been quietly wrong for a while; this PR's UPGRADING.md inherits the same assumption.

Operator impact: an operator unzipping the v2.0 release tarball onto a fresh server hits `web/init.php`'s hard-die ("Compose autoload not found") because there's no vendor at all. They have to figure out `composer install` themselves. The PR's UPGRADING.md tells them this is only a git-pull problem, which is misleading.

Suggested follow-up (out of scope for #1307 strictly speaking — it's a release-pipeline gap that pre-dates this PR):

  • Preferred — fix the workflow to actually ship vendor. Add to `build-webpanel`:
    ```yaml
    - name: Install PHP dependencies
    working-directory: web
    run: composer install --no-dev --optimize-autoloader --no-progress --no-interaction
    ```
    This makes the README and UPGRADING.md claims factually correct and unblocks tarball users.
  • Fallback — clarify UPGRADING.md to acknowledge that BOTH paths require `composer install`. Keeps the doc honest without touching the pipeline, but leaves the README's "bundled" claim still wrong.

Probably worth filing as a separate issue against #1117's release-readiness umbrella. Happy to open it / a fix PR if useful.

Status

Merge stands — the migration half is solid and the doc gap is fixable separately. Flagging here for traceability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

v2.0 upgrade UX gaps: force config.theme=default and document composer install prerequisite

1 participant

@rumblefrog